From Coverity: ** CID 1399676: Error handling issues (CHECKED_RETURN) /obj-x86_64-pc-linux-gnu/dom/bindings/FlyWebWebSocketEventBinding.cpp: 125 in mozilla::dom::FlyWebWebSocketEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::FlyWebWebSocketEvent *, const JSJitMethodCallArgs &)() ________________________________________________________________________________________________________ *** CID 1399676: Error handling issues (CHECKED_RETURN) /obj-x86_64-pc-linux-gnu/dom/bindings/FlyWebWebSocketEventBinding.cpp: 125 in mozilla::dom::FlyWebWebSocketEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::FlyWebWebSocketEvent *, const JSJitMethodCallArgs &)() 119 } 120 binding_detail::FastErrorResult promiseRv; 121 nsCOMPtr<nsIGlobalObject> global = 122 do_QueryInterface(promiseGlobal.GetAsSupports()); 123 if (!global) { 124 promiseRv.Throw(NS_ERROR_UNEXPECTED); >>> CID 1399676: Error handling issues (CHECKED_RETURN) >>> Calling "MaybeSetPendingException" without checking return value (as is done elsewhere 3434 out of 3504 times). 125 promiseRv.MaybeSetPendingException(cx); 126 return false; 127 } 128 arg0 = Promise::Resolve(global, cx, valueToResolve, 129 promiseRv); 130 if (promiseRv.MaybeSetPendingException(cx)) { ** CID 1399677: Error handling issues (CHECKED_RETURN) /obj-x86_64-pc-linux-gnu/dom/bindings/ExtendableEventBinding.cpp: 145 in mozilla::dom::ExtendableEventBinding::waitUntil(JSContext *, JS::Handle<JSObject *>, mozilla::dom::workers::ExtendableEvent *, const JSJitMethodCallArgs &)() ________________________________________________________________________________________________________ *** CID 1399677: Error handling issues (CHECKED_RETURN) /obj-x86_64-pc-linux-gnu/dom/bindings/ExtendableEventBinding.cpp: 145 in mozilla::dom::ExtendableEventBinding::waitUntil(JSContext *, JS::Handle<JSObject *>, mozilla::dom::workers::ExtendableEvent *, const JSJitMethodCallArgs &)() 139 } 140 binding_detail::FastErrorResult promiseRv; 141 nsCOMPtr<nsIGlobalObject> global = 142 do_QueryInterface(promiseGlobal.GetAsSupports()); 143 if (!global) { 144 promiseRv.Throw(NS_ERROR_UNEXPECTED); >>> CID 1399677: Error handling issues (CHECKED_RETURN) >>> Calling "MaybeSetPendingException" without checking return value (as is done elsewhere 3434 out of 3504 times). 145 promiseRv.MaybeSetPendingException(cx); 146 return false; 147 } 148 arg0 = Promise::Resolve(global, cx, valueToResolve, 149 promiseRv); 150 if (promiseRv.MaybeSetPendingException(cx)) { ** CID 1399678: Error handling issues (CHECKED_RETURN) /obj-x86_64-pc-linux-gnu/dom/bindings/FlyWebFetchEventBinding.cpp: 82 in mozilla::dom::FlyWebFetchEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::FlyWebFetchEvent *, const JSJitMethodCallArgs &)() ________________________________________________________________________________________________________ *** CID 1399678: Error handling issues (CHECKED_RETURN) /obj-x86_64-pc-linux-gnu/dom/bindings/FlyWebFetchEventBinding.cpp: 82 in mozilla::dom::FlyWebFetchEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::FlyWebFetchEvent *, const JSJitMethodCallArgs &)() 76 } 77 binding_detail::FastErrorResult promiseRv; 78 nsCOMPtr<nsIGlobalObject> global = 79 do_QueryInterface(promiseGlobal.GetAsSupports()); 80 if (!global) { 81 promiseRv.Throw(NS_ERROR_UNEXPECTED); >>> CID 1399678: Error handling issues (CHECKED_RETURN) >>> Calling "MaybeSetPendingException" without checking return value (as is done elsewhere 3434 out of 3504 times). 82 promiseRv.MaybeSetPendingException(cx); 83 return false; 84 } 85 arg0 = Promise::Resolve(global, cx, valueToResolve, 86 promiseRv); 87 if (promiseRv.MaybeSetPendingException(cx)) { ** CID 1399679: Error handling issues (CHECKED_RETURN) /obj-x86_64-pc-linux-gnu/dom/bindings/FetchEventBinding.cpp: 327 in mozilla::dom::FetchEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::workers::FetchEvent *, const JSJitMethodCallArgs &)() ________________________________________________________________________________________________________ *** CID 1399679: Error handling issues (CHECKED_RETURN) /obj-x86_64-pc-linux-gnu/dom/bindings/FetchEventBinding.cpp: 327 in mozilla::dom::FetchEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::workers::FetchEvent *, const JSJitMethodCallArgs &)() 321 } 322 binding_detail::FastErrorResult promiseRv; 323 nsCOMPtr<nsIGlobalObject> global = 324 do_QueryInterface(promiseGlobal.GetAsSupports()); 325 if (!global) { 326 promiseRv.Throw(NS_ERROR_UNEXPECTED); >>> CID 1399679: Error handling issues (CHECKED_RETURN) >>> Calling "MaybeSetPendingException" without checking return value (as is done elsewhere 3434 out of 3504 times). 327 promiseRv.MaybeSetPendingException(cx); 328 return false; 329 } 330 arg0 = Promise::Resolve(global, cx, valueToResolve, 331 promiseRv); 332 if (promiseRv.MaybeSetPendingException(cx)) {

Boris, do you know what's up here?

Sure. MaybeSetPendingException() returns exactly the same value as Failed(). The idea is that instead of writing out: if (rv.Failed()) { rv.SetPendingException(cx); return; } you do: if (rv.MaybeSetPendingException(cx)) { return; } In the code quoted above, we _KNOW_ promiseRv.Failed() is true: we just called Throw() on it. So there's no point to checking the return value of MaybeSetPendingException. I _could_ make SetPendingException public and use it here, but it's too easy to misuse, so I'd rather not.

Thanks. Sounds like a false positive or at least a P3 for us to change anything.

I have a patch that makes this method MOZ_MUST_USE and fixes a couple of places, including CodeGen, that ignore the return value. They all follow a throw in the same way.

Maybe we should simply have a better API on ErrorResult for "throw this and immediately stick it on the given JSContext".

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5) > Maybe we should simply have a better API on ErrorResult for "throw this and > immediately stick it on the given JSContext". There are few enough of these I don't think it is worth the effort to come up with a new API.

opt and debug try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=312ce1e96a4c5b32fb76be4a43368b2bbed96955 Feel free to grab the review, Boris. I didn't ask because your review requests are closed.

Comment on attachment 8846315 [details] Bug 1338272 - Require that the return value of MaybeSetPendingException is used. https://reviewboard.mozilla.org/r/119278/#review131742 ::: commit-message-d18cc:1 (Diff revision 1) > +Bug 1338272 - Require that the return value of MaybeSetPendingException is used. r=peterv Fix the reviewer here? ;)

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #9) > Fix the reviewer here? ;) I think mozreview actually fixes that up.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5505b53a0acb Require that the return value of MaybeSetPendingException is used. r=bz